integrate libcontainer/userns into moby/sys/user #140

thaJeztah · 2024-07-16T18:04:56Z

relates to libct/userns: split userns detection from internal userns code opencontainers/runc#4331
relates to move most packages into internal opencontainers/runc#3028

integrate libcontainer/userns into moby/sys/user

This integrates the userns package from libcontainer (runc) at commit;
3778ae603c706494fd1e2c2faf83b406e38d687d (1).

Code was originally added through runc@c0ad40c (2), and located in the
libcontainer/system package; history of this code from before it was moved
can be found through 3 and 4.

Migration was done using the following steps:

# install filter-repo (https://github.com/newren/git-filter-repo/blob/main/INSTALL.md)
brew install git-filter-repo

# create a temporary clone of docker
cd ~/Projects
git clone https://github.com/opencontainers/runc.git runc_temp
cd runc_temp

# commit taken from
git rev-parse --verify HEAD
3778ae603c706494fd1e2c2faf83b406e38d687d

git filter-repo --analyze

# remove all code, except for 'libcontainer/userns/', and rename to /user/userns
git filter-repo \
  --path 'libcontainer/userns/userns.go' \
  --path 'libcontainer/userns/userns_linux.go' \
  --path 'libcontainer/userns/userns_fuzzer.go' \
  --path 'libcontainer/userns/userns_linux_fuzzer.go' \
  --path 'libcontainer/userns/userns_linux_test.go' \
  --path 'libcontainer/userns/userns_unsupported.go' \
  --path-rename libcontainer/userns:user/userns

# go to the target github.com/moby/sys repository
cd ~/go/src/github.com/moby/sys

# create a branch to work with
git checkout -b integrate_libcontainer_userns_take2

# add the temporary repository as an upstream and make sure it's up-to-date
git remote add libcontainer_userns ~/Projects/runc_temp
git fetch libcontainer_userns

# merge the upstream code
git merge --allow-unrelated-histories --signoff -S libcontainer_userns/main

thaJeztah · 2024-07-16T20:55:55Z

/cc @AkihiroSuda @kolyshkin @cyphar @lifubang PTAL if this LGTY (related PR in runc is opencontainers/runc#4350)

AkihiroSuda · 2024-07-17T01:06:40Z

Makefile

@@ -16,14 +16,18 @@ clean:
 test: test-local
 	set -eu; \
 	for p in $(PACKAGES); do \
-		(cd $$p; go test $(RUN_VIA_SUDO) -v .); \
+		if $p = user && go version | grep -qv go1.17; then \


No need to support Go <= 1.20

The other modules still have go1.17 as minimum version; I didn't want to change that as part of this PR, but we can revisit that in a follow up

kolyshkin · 2024-07-17T02:57:50Z

user/userns/userns_linux.go

+
+// runningInUserNS detects whether we are currently running in a user namespace.
+//
+// Originally copied from https://github.com/lxc/incus/blob/e45085dd42f826b3c8c3228e9733c0b6f998eafe/shared/util.go#L678-L700.


nit: we should probably refer to runc, too.

Added a mention of runc (both here, and in the package description)

kolyshkin · 2024-07-17T03:16:25Z

It looks like git history here begins when the code was moved in runc, and contains some drastic changes (like additon and removal of a big portion of C code). IOW it's not very useful (I tried to answer simple questions like "why do we use bytes.Buffer?" or "who added the constant 4294967295?" using git blame and git log, and it's not possible to do so).

Given that git history serves a main purpose of figuring out why (or when, or by whom) things were written the way they are written, and that this git history is:

quite lengthy (esp. compared to the amount of code added), yet
doesn't work for the above purpose,

I'm not sure it makes sense to retain it.

I think it's better to just copy and commit the code @ specific commit with a clean git history, and refer to older runc history in the docs (like we did when it was copied from lxc).

thaJeztah · 2024-07-21T13:17:40Z

@kolyshkin I gave this another go, and used more specific filters so that the unrelated commits don't get included. History now contains all history since the code was moved, but the merge-commit contains a link to the commits that moved it.

Let me know if this looks better 🤞

thaJeztah · 2024-07-23T22:48:32Z

@kolyshkin ptal 🤗

cyphar

LGTM

thaJeztah · 2024-07-24T08:09:31Z

~~oh! need to rebase after #142 was merged~~ done! ✅

Moving these utilities to a separate package, so that consumers of this package don't have to pull in the whole "system" package. Looking at uses of these utilities (outside of runc itself); `RunningInUserNS()` is used by [various external consumers][1], so adding a "Deprecated" alias for this. [1]: https://grep.app/search?current=2&q=.RunningInUserNS Signed-off-by: Sebastiaan van Stijn <[email protected]>

For files that end with _linux.go or _linux_test.go, there is no need to specify linux build tag, as it is assumed from the file name. In addition, rename libcontainer/notify_linux_v2.go -> libcontainer/notify_v2_linux.go for the file name to make sense. Signed-off-by: Kir Kolyshkin <[email protected]>

Go 1.17 introduce this new (and better) way to specify build tags. For more info, see https://golang.org/design/draft-gobuild. As a way to seamlessly switch from old to new build tags, gofmt (and gopls) from go 1.17 adds the new tags along with the old ones. Later, when go < 1.17 is no longer supported, the old build tags can be removed. Now, as I started to use latest gopls (v0.7.1), it adds these tags while I edit. Rather than to randomly add new build tags, I guess it is better to do it once for all files. Mind that previous commits removed some tags that were useless, so this one only touches packages that can at least be built on non-linux. Brought to you by go1.17 fmt ./... Signed-off-by: Kir Kolyshkin <[email protected]>

This makes libcontainer/userns self-dependent, largely returning to the original implementation from lxc. The `uiMapInUserNS` is kept as a separate function for unit-testing and fuzzing. Signed-off-by: Sebastiaan van Stijn <[email protected]>

Removed pre-go1.17 build-tags with go fix; go fix -mod=readonly ./... Signed-off-by: Sebastiaan van Stijn <[email protected]>

This was a poor decision on my side; ab29593 moved this utility to a separate package, and split the exported function from the implementation (and stubs). Out of convenience, I used an alias for the latter part, but there's two downsides to that; - `RunningInUserNS` being an exported var means that (technically) it can be replaced by other code; perhaps that's a "feature", but not one we intended it to be used for. - `RunningInUserNS` being implemented through a var / alias means it's also documented as such on [pkg.go.dev], which is confusing. This patch changes it to a regular function, acting as a wrapper for the underlying implementations. While at it, also slightly touching up the GoDoc to describe its functionality / behavior. [pkg.go.dev]: https://pkg.go.dev/github.com/opencontainers/[email protected]/libcontainer/userns#RunningInUserNS Signed-off-by: Sebastiaan van Stijn <[email protected]>

The fuzzer for this only runs on Linux; rename the file to be Linux-only so that we don't have to stub out the uidMapInUserNS function. Signed-off-by: Sebastiaan van Stijn <[email protected]>

Now that we dropped support for go < 1.21, we can use this; moving the sync.once out of the runningInUserNS() implementation would also allow for it to be more easily tested if we'd decide to. Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

laurazard

LGTM

tianon · 2024-07-25T15:15:48Z

I'm a little late to the party (sorry 😭), but I'm actually a bit confused by this. As far as I can tell, the only thing the "user" and "userns" packages have in common is the name, so this seems like an odd grouping IMO

tianon · 2024-07-29T17:55:49Z

I'm also missing the justification for the bump to go1.21? 5cd502c#r144637064

(and in fact, #140 (comment) seems to directly contradict doing so 🤔)

thaJeztah · 2024-07-30T17:41:03Z

@tianon Arf, sorry for ghosting you; I saw your comment on my phone, and wanted to bring it up in one of the calls, but I didn't make both of them, then forgot.

I'm a little late to the party (sorry 😭), but I'm actually a bit confused by this. As far as I can tell, the only thing the "user" and "userns" packages have in common is the name, so this seems like an odd grouping IMO

That, hm, is a valid point. I think I initially put it as a separate module, but then (maybe incorrectly?) recalled that there was a discussion to move this, as well as possibly the idtools bit into the same module. But maybe I my memory fails me here (it's been leaky a bit recently).

FWIW; to my knowledge none of the current consumers (moby, buildkit, containerd) have included this change in a release, so there's still a potential to change this to a separate module if we think that's better

I'm also missing the justification for the bump to go1.21? 5cd502c#r144637064

The code in userns uses sync.OnceFunc, which requires go1.21

(and in fact, #140 (comment) seems to directly contradict doing so 🤔)

Ah, right, so the comment was about the added check in make test; GitHub actions runs on go1.18, go1.21.x and go1.22.x; the lines added are to skip the user module on go1.18, but still test the other modules (mount, mountinfo, sequential, signal, symlink) on that version.

tianon · 2024-07-30T18:58:42Z

The context for why I even care is that I'm a downstream user of github.com/moby/sys/user who will never touch userns, so this change has me strongly considering the Go Proverb "A little copying is better than a little dependency." (but "a little dependency" is part of why github.com/moby/sys/user was created / exists in the first place) 😅 🙈

tianon · 2024-07-30T18:59:03Z

So that's the long way of saying I'm very much in favor of a git mv user/userns userns

thaJeztah · 2024-08-06T11:21:51Z

I opened a PR to move this to a separate module;

user: move userns package to separate module, and retract v0.2.0 #145

❯ go get github.com/containers/[email protected] go: warning: github.com/moby/sys/[email protected]: retracted by module author: Package github.com/moby/sys/user/userns was included in this module, but should've been a separate module; see moby/sys#140 (comment). go: to switch to the latest unretracted version, run: go get github.com/moby/sys/user@latest go: downgraded github.com/containers/storage v1.55.0 => v1.53.0 ~/git/openshift-velero-plugin remotes/weshayutin/konveyor_dev_CVE-2024-3727* ❯ go mod tidy ~/git/openshift-velero-plugin remotes/weshayutin/konveyor_dev_CVE-2024-3727* ❯ go vet ./... Signed-off-by: Tiger Kaovilai <[email protected]>

….2 (#288) * CVE-2024-3727 GO-2024-2842: bump github.com/containers/image/v5 v5.30.2 Signed-off-by: Wesley Hayutin <[email protected]> * Fix storage import vet failure. ❯ go get github.com/containers/[email protected] go: warning: github.com/moby/sys/[email protected]: retracted by module author: Package github.com/moby/sys/user/userns was included in this module, but should've been a separate module; see moby/sys#140 (comment). go: to switch to the latest unretracted version, run: go get github.com/moby/sys/user@latest go: downgraded github.com/containers/storage v1.55.0 => v1.53.0 ~/git/openshift-velero-plugin remotes/weshayutin/konveyor_dev_CVE-2024-3727* ❯ go mod tidy ~/git/openshift-velero-plugin remotes/weshayutin/konveyor_dev_CVE-2024-3727* ❯ go vet ./... Signed-off-by: Tiger Kaovilai <[email protected]> --------- Signed-off-by: Wesley Hayutin <[email protected]> Signed-off-by: Tiger Kaovilai <[email protected]> Co-authored-by: Tiger Kaovilai <[email protected]>

thaJeztah force-pushed the integrate_libcontainer_userns branch 2 times, most recently from 6438200 to 7cfea2c Compare July 16, 2024 18:21

thaJeztah marked this pull request as ready for review July 16, 2024 20:54

thaJeztah requested a review from kolyshkin July 16, 2024 20:54

AkihiroSuda reviewed Jul 17, 2024

View reviewed changes

AkihiroSuda approved these changes Jul 17, 2024

View reviewed changes

kolyshkin reviewed Jul 17, 2024

View reviewed changes

thaJeztah force-pushed the integrate_libcontainer_userns branch 2 times, most recently from 7b76d4a to aa1d8ad Compare July 21, 2024 13:15

thaJeztah force-pushed the integrate_libcontainer_userns branch from aa1d8ad to 6d16ea0 Compare July 22, 2024 07:46

thaJeztah requested a review from kolyshkin July 22, 2024 16:37

lifubang approved these changes Jul 23, 2024

View reviewed changes

cyphar approved these changes Jul 24, 2024

View reviewed changes

thaJeztah and others added 7 commits July 24, 2024 10:10

remove pre-go1.17 build-tags

bb72464

Removed pre-go1.17 build-tags with go fix; go fix -mod=readonly ./... Signed-off-by: Sebastiaan van Stijn <[email protected]>

libct/userns: make fuzzer Linux-only, and remove stub for uidMapInUserNS

bc0de32

The fuzzer for this only runs on Linux; rename the file to be Linux-only so that we don't have to stub out the uidMapInUserNS function. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah added 3 commits July 24, 2024 10:10

user/userns: add godoc for package

a40602b

Signed-off-by: Sebastiaan van Stijn <[email protected]>

user: require go1.21 or higher

5cd502c

Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the integrate_libcontainer_userns branch from 6d16ea0 to 5cd502c Compare July 24, 2024 08:12

thaJeztah self-assigned this Jul 24, 2024

laurazard approved these changes Jul 25, 2024

View reviewed changes

laurazard merged commit 86870e7 into moby:main Jul 25, 2024
19 checks passed

thaJeztah deleted the integrate_libcontainer_userns branch July 25, 2024 08:53

This was referenced Jul 25, 2024

migrate to github.com/moby/sys/user/userns moby/buildkit#5189

Merged

[27.1 backport] migrate to github.com/moby/sys/user/userns moby/moby#48238

Merged

This was referenced Aug 1, 2024

Migrate per-endpoint sysctls until 28.0.0 moby/moby#48281

Merged

user: move userns package to separate module, and retract v0.2.0 #145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrate libcontainer/userns into moby/sys/user #140

integrate libcontainer/userns into moby/sys/user #140

thaJeztah commented Jul 16, 2024 •

edited

Loading

thaJeztah commented Jul 16, 2024

AkihiroSuda Jul 17, 2024

thaJeztah Jul 17, 2024

kolyshkin Jul 17, 2024

thaJeztah Jul 21, 2024

kolyshkin commented Jul 17, 2024

thaJeztah commented Jul 21, 2024

thaJeztah commented Jul 23, 2024

cyphar left a comment

thaJeztah commented Jul 24, 2024 •

edited

Loading

laurazard left a comment

tianon commented Jul 25, 2024 •

edited

Loading

tianon commented Jul 29, 2024

thaJeztah commented Jul 30, 2024

tianon commented Jul 30, 2024

tianon commented Jul 30, 2024

thaJeztah commented Aug 6, 2024

integrate libcontainer/userns into moby/sys/user #140

integrate libcontainer/userns into moby/sys/user #140

Conversation

thaJeztah commented Jul 16, 2024 • edited Loading